Skip to content

feat: add ERROR detail to output of migration#1142

Merged
WikiRik merged 1 commit into
sequelize:mainfrom
snewcomer:sn/error-detail
Sep 27, 2022
Merged

feat: add ERROR detail to output of migration#1142
WikiRik merged 1 commit into
sequelize:mainfrom
snewcomer:sn/error-detail

Conversation

@snewcomer
Copy link
Copy Markdown
Contributor

@snewcomer snewcomer commented Aug 20, 2022 •
edited
Loading

Pull Request check-list

  • Does npm run test pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

close #1138

This PR adds additional error output. Currently the error output may be insufficient to hone in on the problem.

https://www.postgresql.org/docs/current/plpgsql-errors-and-messages.html

Desired behaviour -

ERROR: SequelizeForeignKeyConstraintError: insert or update on table "my_table" violates foreign key constraint "...."
ERROR DETAIL: Key (my_column)=(1) is not present in table "my_table"

Alternatives

Behind a debug flag. My initial hunch is to consider this a helpful feature that users get out of the box. Happy to put behind a debug flag though!

@WikiRik
Copy link
Copy Markdown
Member

WikiRik commented Aug 21, 2022

Hi! Thanks for the PR! I haven't given the alternative more thought yet, but can you add one or more tests for this? More tests is especially preferred if on some cases more information is given that might be considered confidential

@snewcomer
Copy link
Copy Markdown
Contributor Author

snewcomer commented Sep 5, 2022

@WikiRik Gave a quick look around and not seeing a good extension point for this test. Do you happen to know of a place?

@fzn0x
Copy link
Copy Markdown
Member

fzn0x commented Sep 27, 2022

The changes just returning void and hard to test, maybe to test the detail property existence? @WikiRik

fzn0x
fzn0x approved these changes Sep 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@fzn0x fzn0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to have this log

@WikiRik
Copy link
Copy Markdown
Member

WikiRik commented Sep 27, 2022

@snewcomer sorry for not responding before, I took a look as well and we do not really have any tests to check our error behaviour. We'll add that in a later stage. For now, this is fine

Thanks for the PR!

@WikiRik WikiRik merged commit 497b805 into sequelize:main Sep 27, 2022
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 27, 2022

🎉 This PR is included in version 6.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fzn0x fzn0x fzn0x approved these changes

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add DETAIL to migration error output

3 participants

@snewcomer @WikiRik @fzn0x